-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
new driver for the lis2de12 accelerometer #20162
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome and thank you for the PR!
The driver looks good, but static-checks has some suggestions.
It looks like some features (interrupt mode, fifo) alre only halfway implemented. E.g. you configure the interrupt mode and fifo, but never read the fifo or register an interrupt pin for the interrupt line.
Do you plan on adding those features to the PR?
And is lis2dh12
sufficiently different from lis2de12
that a common driver is not feasible?
I see they both even respond with the same WHO_AM_I_VAL
response in the driver init, so wouldn't lis2dh12
already work with this device out of the box if you set LIS2DH12_PARAM_RESOLUTION
to LIS2DH12_POWER_LOW
as it seems that the only difference is that lis2de12
only supports 8-bit mode.
* @{ | ||
* | ||
* @file | ||
* @brief Command definition for the LIS2DE12 accelerometer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might as well be in lis2de12_registers.h
fifo_reg.bit.TR = config->FIFO_set_INT2; | ||
fifo_reg.bit.FM = config->FIFO_mode; | ||
fifo_reg.bit.FTH = config->FIFO_watermark; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are configuring the interrupt on the device side, but there is no interrupt handling in the driver.
/* read sampled data from the device */ | ||
_acquire(dev); | ||
|
||
float sensitivity = (((_read(dev, REG_CTRL_REG4) >> 4) & 0x3) + 1) * 15.6f; //0: +-2g |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the 15.6
come from?
int lis2de12_set_scale(lis2de12_t *dev, lis2de12_scale_t scale); | ||
lis2de12_scale_t lis2de12_get_scale(lis2de12_t *dev); | ||
|
||
int lis2de12_set_fifo(const lis2de12_t *dev, const lis2de12_fifo_t *config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are never doing anything with the fifo, you only read the first element with lis2de12_read()
Contribution description
This new code is based on the existing lisdh12 driver. The lis2dh12 and lis2de12 are very similar.
Testing procedure
I made a test example for this drivers in test/drivers/lis2de12
This test can: